This is a big challenging to implement concurrent-safe, will implement in a separate PR. As this module doesn't need it yet, will add along with modules that need it. For now I'm leaving it as a TODO item.

I'm wondering if we even need to implement it? I don't see any usage of DeleteRange in our code-base right now.

I assume we will want to add them later. But maybe we can:

• do the migration with the common writer interface omitting DeleteRange
• when the migration is complete, re-add DeleteRange to the writer interface (then we don't need to implement it with Badger)

OK, I can remove it for now.

We need the DeleteRange method for pebble to remove all events, transactions etc for a given block when rolling back executed height.

I used Pebble's DeleteRange method as it can iterate range of keys with certain prefix and delete them atomically.

Once we're done prototyping and align on a structure we expect to be long-lived (maybe now?), we should document these callback functions, for example:

• The error input is the error returned from batch.Commit
• The callback is called regardless of whether the batch was successfully committed (callbacks are responsible for verifying this by checking the error input)
• Callbacks must be non-blocking

Realistically we will need to keep this structure where the ReaderBatchWriter passed to lower-level storage methods can both read and write, even though it doesn't fit well conceptually with Pebble. I think it would be worthwhile to make the interface naming and documentation clearly communicate the fact that the reader is reading globally from committed database state.

I also feel it would be better to separate the method to access reader and writer:

• we want the design to discourage intermingling reads and writes, and communicate the fact that reads and writes are not being applied to the same snapshot of state like they were with badger.
• having one return value allows chaining (eg. batch.Reader().Get(...))
• separating the functions allows us to distinctly document each one (since the reader and writer are fairly conceptually different)

// GlobalReader returns a database-backed reader which reads the latest committed global database state ("read-committed isolation").
// This reader will not read writes written to ReaderBatchWriter.Writer until the write batch is committed.
// This reader may observe different values for the same key on subsequent reads. 
func (b *ReaderBatchWriter) GlobalReader() storage.Reader { ... }

// Writer returns a writer associated with a batch of writes. The batch is pending until it is committed.
// When we `Write` into the batch, that write operation is added to the pending batch, but not committed.
// The commit operation is atomic w.r.t. the batch; either all writes are applied to the database, or no writes are.
func (b *ReaderBatchWriter) Writer() storage.Writer { ... }

Makes sense.

I originally combined them because there is a few places need intermingling reads. But I think we probably are able to refactor those logic, such as bootstrapping a sealing segment.

Implemented storage.OnCommitSucceed.

Far from the highest priority, but it would be great if we could replace "transaction" terminology with "reader" or "writer" terminology where we notice it in new code.

I'd like to communicate which low-level functions must be synchronized by a higher-level storage procedure. Adding documentation and using Unsafe... naming seems like a relatively easy way to do that.

Before merging, please add documentation for these and other public methods. I wrote a version in this comment for the reader/writer methods: #6381 (comment).